Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom nth_back for Chain #60492

Merged
merged 1 commit into from
Aug 16, 2019
Merged

Conversation

acrrd
Copy link
Contributor

@acrrd acrrd commented May 2, 2019

Implementation of nth_back for Chain.
Part of #54054

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2019
@acrrd
Copy link
Contributor Author

acrrd commented May 2, 2019

r? @scottmcm

@koalatux
Copy link
Contributor

koalatux commented May 6, 2019

Nice! Is it possible with the current state of default impl to also have a more specific implementation, where B also implements ExactSizeIterator?

@acrrd
Copy link
Contributor Author

acrrd commented May 7, 2019

What is the best way to test both implementations?

@Dylan-DPC-zz
Copy link

ping from triage @scottmcm waiting for your review on this

@timvermeulen
Copy link
Contributor

You're not mutating self.b in any way when n >= self.b.len() in the specialized version, so if the state is ChainState::Back, I don't think calling nth_back with a too large value will actually empty the iterator (even though it will return None). I haven't tested it, but I think this will fail:

let mut iter = std::iter::empty().chain(vec![0, 0]);
iter.next();        // to change the state to ChainState::Back
iter.nth_back(100); // to empty the iterator
assert_eq!(iter.next(), None);

I think you'll need to call self.b.nth_back(n) regardless of the length of b.

@acrrd
Copy link
Contributor Author

acrrd commented May 28, 2019

@timvermeulen I added tests for that and fixed the specialization. Thanks!

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Jun 18, 2019

Ping from triage, still waiting on your review @scottmcm

(or someone else from @rust-lang/libs)

@scottmcm
Copy link
Member

scottmcm commented Jul 2, 2019

This is introducing a default fn on an Iterator impl that's not there in the analogous forward implementation of the method, so I'd like someone from libs to take this. *rolls dice*

r? @Kimundi

@rust-highfive rust-highfive assigned Kimundi and unassigned scottmcm Jul 2, 2019
@timvermeulen
Copy link
Contributor

Presumably there could also be a specialized nth when A: ExactSizeIterator?

@acrrd acrrd force-pushed the issues/54054_chain branch 2 times, most recently from b625514 to 558f909 Compare July 2, 2019 10:35
@acrrd
Copy link
Contributor Author

acrrd commented Jul 8, 2019

What could be the problem in not having nth also be specialized for ExactSizeIterator?

@totsteps
Copy link
Member

ping from triage, @Kimundi @Centril any updates on this?

@fmckeogh
Copy link
Member

Second ping from wg-triage, pinging random member of T-libs per procedure.

@SimonSapin

@Dylan-DPC-zz
Copy link

r? @SimonSapin

@rust-highfive rust-highfive assigned SimonSapin and unassigned Kimundi Aug 1, 2019
@SimonSapin
Copy link
Contributor

Per discussion in #62483 (comment) it’s not clear that we want to use specialization in public trait (as opposed to a private trait that only optimize an implementation detail) while specialization itself is still unstable, or at least while it has significant design issue still open.

CC @alexcrichton

@alexcrichton
Copy link
Member

I'm personally not the biggest fan of so aggressively using specialization myself, but at a bare minimum the general convention we've had (or so I think) is that all specialization is done internally in the standard library and no public-facing function is listed as default

@JohnCSimon
Copy link
Member

Ping from triage @acrrd @SimonSapin
Hi! This has sat idle for awhile. Is this close to being resolved?

@acrrd
Copy link
Contributor Author

acrrd commented Aug 11, 2019

After the comment of @SimonSapin and @alexcrichton I removed the specialization

@SimonSapin
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 16, 2019

📌 Commit 7b6ad60 has been approved by SimonSapin

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 16, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 16, 2019
Add custom nth_back for Chain

Implementation of nth_back for Chain.
Part of rust-lang#54054
bors added a commit that referenced this pull request Aug 16, 2019
Rollup of 10 pull requests

Successful merges:

 - #60492 (Add custom nth_back for Chain)
 - #61780 (Finalize the error type for `try_reserve`)
 - #63495 ( Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.)
 - #63525 (Make sure that all file loading happens via SourceMap)
 - #63595 (add sparc64-unknown-openbsd target)
 - #63604 (Some update for vxWorks)
 - #63613 (Hygienize use of built-in macros in the standard library)
 - #63632 (A couple of comment fixes.)
 - #63634 (ci: properly set the job name in CPU stats)
 - #63636 (ci: move linkcheck from mingw-2 to mingw-1)

Failed merges:

r? @ghost
@bors bors merged commit 7b6ad60 into rust-lang:master Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.